Skip to content

feat: expose comet metrics through Sparks external monitoring system#3708

Open
coderfender wants to merge 6 commits intoapache:mainfrom
coderfender:project_comet_metrics
Open

feat: expose comet metrics through Sparks external monitoring system#3708
coderfender wants to merge 6 commits intoapache:mainfrom
coderfender:project_comet_metrics

Conversation

@coderfender
Copy link
Contributor

@coderfender coderfender commented Mar 15, 2026 •
edited
Loading

Which issue does this PR close?

Closes #3712

Rationale for this change

  1. We currently collect metrics and show while logging in comet. This PR is to essentially go a step further and wire the metrics out to be visualized in comet through tools like Grafana . As more users start using comet, these metrics would be helpful in understanding acceleration from comet standpoint. These are first Comet metrics exposed to Spark's external monitoring system and more would follow

The following metrics are exposed at the moment :

  1. comet.operators.native
  2. comet.operators.spark
  3. comet.queries.planned
  4. comet.transitions
  5. comet.acceleration.ratio (acceleration : native / (native + spark)

What changes are included in this PR?

How are these changes tested?

@coderfender coderfender force-pushed the project_comet_metrics branch from aa97dec to f096d8f Compare March 15, 2026 22:02
@coderfender
Copy link
Contributor Author

coderfender commented Mar 15, 2026

cc : @andygrove

@coderfender coderfender changed the title feat: expose comet metrics for visualization feat: expose comet metrics through Sparks external monitoring system Mar 16, 2026
wForget
wForget reviewed Mar 16, 2026
View reviewed changes
@coderfender coderfender force-pushed the project_comet_metrics branch from f096d8f to c0eb149 Compare March 16, 2026 03:03
@coderfender
Copy link
Contributor Author

coderfender commented Mar 16, 2026

@wForget , Please take a look whenever you get a chance

@coderfender coderfender force-pushed the project_comet_metrics branch from c0eb149 to 2eb506f Compare March 16, 2026 15:52
wForget
wForget reviewed Mar 17, 2026
View reviewed changes
@coderfender coderfender force-pushed the project_comet_metrics branch from 8c60dc8 to eaa1dc4 Compare March 17, 2026 08:10
@coderfender
Copy link
Contributor Author

coderfender commented Mar 17, 2026

@wForget , I addressed code per your review comments. Please take a look whenever you get a chance. Thank you for the kind guidance

wForget
wForget reviewed Mar 17, 2026
View reviewed changes
wForget
wForget reviewed Mar 17, 2026
View reviewed changes
parthchandra
parthchandra reviewed Mar 17, 2026
View reviewed changes
spark/src/main/scala/org/apache/spark/CometSource.scala
}
})

def recordStats(stats: CometCoverageStats): Unit = {
Copy link
Contributor

@parthchandra parthchandra Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't exactly ideal. These stats make sense per plan. Here we are incrementing all metrics globally, so all plans that are executed across all sessions in a Spark application will have their counts accumulated and may provide no useful information.

Copy link
Contributor Author

@coderfender coderfender Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. But spark does follow a similar approach here but there is always an option to improve granularity

Copy link
Contributor

@parthchandra parthchandra Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't saying the approach is wrong. Just pointing out that for these specific metrics, if the Spark application has multiple plans, the accumulated values are not necessarily meaningful.

@coderfender
Copy link
Contributor Author

coderfender commented Mar 18, 2026

Thwnk you for the review . Let me take a look and update the logic

@coderfender coderfender force-pushed the project_comet_metrics branch from c6da902 to 2b6dcc1 Compare March 18, 2026 22:06
wForget
wForget reviewed Mar 20, 2026
View reviewed changes
spark/src/main/scala/org/apache/spark/Plugins.scala
}

object CometDriverPlugin extends Logging {
def registerCometMetrics(sc: SparkContext): Unit = {
Copy link
Member

@wForget wForget Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a configuration to enable this behavior.

@wForget
Copy link
Member

wForget commented Mar 20, 2026

Thanks @coderfender , overall looks good to me; just need to add a configuration to control this behavior, and keep it disabled by default.

@coderfender
Copy link
Contributor Author

coderfender commented Mar 20, 2026 •
edited
Loading

@wForget , thank you for the approval. My preference here is to enable metrics reporting by default unless you think that we would be compromising performance in collecting the metrics ?

@wForget
Copy link
Member

wForget commented Mar 23, 2026

My preference here is to enable metrics reporting by default unless you think that we would be compromising performance in collecting the metrics ?

  1. Spark metrics are not always collected; they typically require a metrics system sink like Prometheus.
  2. Spark plan traversal is not lightweight enough, they may have many nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parthchandra parthchandra parthchandra left review comments

@wForget wForget wForget left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Export comet metrics externally through spark's monitoring system

3 participants

@coderfender @wForget @parthchandra